Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for open_basedir before reading /proc #37959

Merged
merged 2 commits into from
Nov 15, 2023
Merged

Check for open_basedir before reading /proc #37959

merged 2 commits into from
Nov 15, 2023

Conversation

solracsf
Copy link
Member

@solracsf solracsf commented Apr 27, 2023

Summary

Check if open_basedir is set. If yes and is restrictive, or if can't be determined, set it to 0.

Checklist

@solracsf solracsf added the 3. to review Waiting for reviews label Apr 27, 2023
@solracsf solracsf added this to the Nextcloud 27 milestone Apr 27, 2023
@szaimen szaimen requested review from a team, ArtificialOwl, icewind1991, blizzz and kesselb and removed request for a team April 27, 2023 15:24
$width = is_readable('/proc/cpuinfo') ? substr_count(file_get_contents('/proc/cpuinfo'), 'processor') : 0;
} else {
$openBasedirPaths = explode(':', $openBasedir);
foreach ($openBasedirPaths as $path) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should also work to use strpos /proc and strpos /proc/cpuinfo on openbasedir without the explode 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong. To check properly for /proc and/or /proc/cpuinfo we indeed need to iterate over the entries and make sure to detect if the whole folder is allowed.

@kesselb
Copy link
Contributor

kesselb commented Apr 27, 2023

I think we should remove getHardwareConcurrency and use a sane default.

A hint for preview_concurrency_all and preview_concurrency_new at https://docs.nextcloud.com/server/latest/admin_manual/installation/server_tuning.html would be nice.

Every time a generator instance is loaded, we fetch /proc/cpuinfo to use the number of processor as default value. I think this is unnecessary.

We don't support FreeBSD. But TrueNAS which is based on FreeBSD. On FreeBSD /proc/cpuinfo is not available. We need a different way to retrieve the processors: https://github.com/nextcloud/serverinfo/blob/1146b21d13073c5e8799eb707096d12b5f682026/lib/OperatingSystems/FreeBSD.php#L81

My recommendation is to keep it simple.

@joshtrichards
Copy link
Member

This whole section of code getHardwareConcurrency() is semi-irrelevant anyhow since there are defaults defined and the admin can also define the best (preferred) parameters via preview_concurrency_* already. This is just fallback code and shouldn't be getting executed every time.

The overall generator CPU balancing feature is sound, but the code elsewhere within it could use some refactoring.

The only reason the open_basedir trigger in this issue can't easily be headed off by the admin simply configuring the preview_concurrency_* settings is because of the order of operations the fallback is implemented in.

I also wouldn't bother with trying to process the contents of the open_basedir if it's found. I'd just fall back to the sane defaults (which the admin can still override if they wish as currently coded). Plus even after parsing open_basedir the processing of /proc could still fail for other reasons (like different OSes as @kesselb noted).

That said, K.I.S.S. - and rather than submitting a competing PR - here's the quick and dirty path I suggest until a more extensive refactor can happen:

joshtrichards@30c5f24

@solracsf
Copy link
Member Author

solracsf commented Apr 27, 2023

I agree that simplication is (almost) ALWAYS the way to go, but this PR is only about open_basedir error suppression, nothing more. Feel free to submit a proper PR to simplify the whole thing or complete the actual checks for all OSes.

Copy link
Member

@joshtrichards joshtrichards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay to me. Achieves the primary aim: allow some folks to add it to their open_basedir, but if they can't (or don't) it'll fall through to the defaults while still also permitting an explicit override to more appropriate local parameters by way of the preview_concurrency_* config parameters. And all without erroring out.

There are still some edge cases like if open_basedir = "/tmp:/home/user:/proc" which technically does permit cpuinfo access but would still fail this test, but given the above I don't think that matters nor is worth complicating the code further (plus it'd be sort of weird to lock things down with open_basedir yet then open up all of /proc IMO, but I've seen loads of weird things in the wild hah).

This was referenced May 3, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@taziobaker
Copy link

taziobaker commented Oct 12, 2023

Could this work?:

public static function getHardwareConcurrency(): int {
static $width;
if (!isset($width)) {
if (function_exists('nproc')) {
// 'nproc' function is available, use it to determine the hardware concurrency
$width = (int) nproc();
} elseif (strtoupper(substr(PHP_OS, 0, 3)) === 'LIN') {
// Use 'sysctl' to query the number of CPU cores on Linux
$result = shell_exec('sysctl -n hw.ncpu');
if ($result !== null) {
$width = (int) trim($result);
} else {
$width = 0; // Handle the case when 'sysctl' is not available or fails.
}
} else {
// Handle other platforms or cases
$width = 0;
}
}
return $width;
}

This first checks if the nproc function is available. If it is, it uses nproc to determine the hardware concurrency. If nproc is not available and the operating system is Linux, it uses the sysctl approach to query the number of CPU cores. If neither method is available or if the code is running on a platform other than Linux, it sets the hardware concurrency to 0.

Just my ¢2

@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
@blizzz blizzz mentioned this pull request Nov 14, 2023
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
@szaimen szaimen merged commit c2658bf into master Nov 15, 2023
50 checks passed
@szaimen szaimen deleted the ob-proc branch November 15, 2023 15:48
@szaimen
Copy link
Contributor

szaimen commented Nov 16, 2023

/backport to stable27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: flood of errors is_file(): open_basedir restriction in effect. File(/proc/cpuinfo) after 25 > 26 update
7 participants